-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BLD: Build shared c dependencies as a library #47081
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -16,5 +16,4 @@ runs: | |||
python -m pip install -e . --no-build-isolation --no-use-pep517 --no-index | |||
shell: bash -el {0} | |||
env: | |||
# Cannot use parallel compilation on Windows, see https://github.com/pandas-dev/pandas/issues/30873 | |||
N_JOBS: ${{ runner.os == 'Windows' && 1 || 2 }} | |||
N_JOBS: 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might still be worth aligning this with the number of core on the GH hosted runners (2 Window/Linux, 3 MacOS): https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources=
Does this affect build size or speed or inlining or ...? |
I like this PR, and want to try it out, but my laptop where I do 8-core builds on Windows that always fails is out of commission for a week or so. Once I get that back, I will give it a try and let you know the outcome. |
setup.py
Outdated
], | ||
"include_dirs": [ | ||
"pandas/_libs/tslibs/src/datetime", | ||
sysconfig.get_path("include"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these get_path("include")
calls? I think the compiler should handle that natively
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, the compiler was not able to find Python.h without this.
…into c-shared-library
@lithomas1 do you know what makes the compilation process different for these versus shared libraries created from compiled Cython modules? I'm slightly -1 on adding this complexity and certainly -1 for expanding it beyond these libraries for what seems like a very marginal gain |
I just tried this PR to build the latest version on my Windows laptop using |
The problem is that there seems to be no way to link against other Extensions(the shared library made from the Cython module) from another Extension. The first option from SO Link is what I'm doing (the other options don't seem to be too relevant, and are also going over my head). The other benefit of this is that the parser and the tslibs c sources don't get recompiled many times. |
Would this potentially help solve #47305? |
Does the |
ok let's merge this and see |
I'm pretty sure I tried this and it didn't work, let me double check. |
I am fine with this for now if it fixes immediate issues. Can always clean up later. Also exploring different build tooling in #47380 which could help |
So I upgraded my Visual Studio installation to Visual Studio 2019, and tried this PR, using Note that using Having said that, I think I know what the issue is, which is that there were still references to |
@@ -16,7 +16,4 @@ runs: | |||
python -m pip install -e . --no-build-isolation --no-use-pep517 --no-index | |||
shell: bash -el {0} | |||
env: | |||
# Cannot use parallel compilation on Windows, see https://github.com/pandas-dev/pandas/issues/30873 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this address's @simonjayhawkins #47081 (comment) I would prefer to keep this at N_JOBS: 1
Otherwise, I would suggest this be specific to the number of cores available on GHA hosted runners by OS: #47081 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this address's @simonjayhawkins #47081 (comment)
I've needed to restart a few workflows this last week or so as they fail on the initial build, but it maybe once it is fixed that the initial build is successful, that subsequent incremental builds for commits in the range of the last couple of weeks could also be successful.
And it could also be that if this is a reoccurring problem that I can change the workflow to repeat the previous build step if the pandas import fails.
so I am happy that any comments/concerns are fully addressed before merging this.
@lithomas1 see my suggested changes in lithomas1#16 |
I haven't touched this PR in a while, since we are considering Meson(working on exploring this front)/Cmake as our new build system. If the other core devs still think this is worthwhile, I am happy to pick it back up though. |
@lithomas1 I'm happy if you are working on different approach instead. xref #47081 (comment) I don't recall seeing |
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
It might be time to revive this. This might fix the CI and I don't think the meson work will be ready before at least the end of this year. cc @WillAyd |
Hmm I think I'd prefer not to hack setuptools like this versus just using Meson. Stuck with anything in particular on that I might be able to help with? |
FWIW if you look at the build log this still generates the |
See my message above: #47081 (comment) I created a PR for @lithomas1 that fixes this issue. |
Hmm interesting. It looks like the build log shows that file still getting generated a few times. Not opposed to merging this as is just don't think this does what we think it does. Just want to be wary of continued setuptools hacks going forward |
I working out a couple of bugs in meson and meson-python. mesonbuild/meson#10639 is the one I'm looking into right now.
Merged it, thanks.
Yeah, this could end up not worth it ifit causes a performance egression somewhere or somehow borks the wheels. |
Around 50% of the time I have to rebuild due to the undefined symbol issue with
three times all builds were successful. Doing the same on main, all three failed. Based on the conversation above, I think that makes me +32 on this PR. |
Small update on this: Thanks all for the encouragement on this, and sorry for the long period without updates. Work on meson is a mixed bag
The bad news is I still don't think we'll be able to get meson enabled as the default build system before the end of the year. I've been upstreaming fixes to both the meson and meson-python libraries, and probably is going to be a while before they cut another release. The good news is that we'll probably have some sort of meson support in main by next month(hopefully?) for brave people to try, it just won't be in the CI until meson and meson-python cut a new release with my fixes in them. |
This is managed today via versioneer, which I think is only supported via setuptools. Might need to look at upstreaming support for meson in that as well |
Re the version, it depends on what you want:
(2) isn't possible easily in Meson right now, because using its (3) seems to be more feasible. SciPy used to do (2), and I switched it to (1) during the move to Meson. That has been fine so far. The benefit of having the git hash in the version is relatively minor, and it's still available at install time as |
I'm a little worried that this will overwrite nightly wheels, though, since the filenames will be the same. This could be a problem if someone downstream is pinning the tests of pandas nightlies. Is there something we can do in meson-python to workaround this? Since the version in meson has to be hard-coded, it might make sense for meson-python to take an option in the pyproject.toml, to get the version from git? |
That sounds like a reasonable thing to do in
So perhaps rather than baking it into |
Will have to excuse any ignorance in the question but is it possible to continue to use setuptools with meson? |
It's perfectly fine to have two build systems live side-by-side in this repo. SciPy has had both |
If that's the case I would think it's probably easier to just stick with setuptools for the initial cutover then change to meson-Python at a later date. At the very least that way can move forward without having to solve the versioning issue |
Actually re-reading your comment I think we are talking about different things, but moved reply to lithomas1#19 (comment) |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Does anyone else want to test this? I can only test with -j4 on my old 2-core i3.